-
Notifications
You must be signed in to change notification settings - Fork 470
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add UNOFFICIAL
category for the United States
#1669
Conversation
Pull Request Test Coverage Report for Build 8106192363Details
💛 - Coveralls |
Note: due to the current United States implementation, |
While we don't have NON_PUBLIC subdiv holidays, we can just add if PUBLIC not in self.categories:
return None to the beginning of When such holidays appear, we should change it to if PUBLIC in self.categories:
# MLK Day ... |
Happy Groundhog Day! 🎉😉 |
It turns out from some checks they do celebrate this in Hawaii, too, instead of just the Continental U.S. huh, even if they didn't have any groundhogs there 👀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. This category (NON PUBLIC) opens up a very wide field of activity for us... ;)
If there any additional changes required for this PR? 👀 |
No, not really. I was just thinking about |
I also like |
6cb297e
to
79b513b
Compare
NON_PUBLIC
category for the United States.UNOFFICIAL
category for the United States.
Quality Gate passedIssues Measures |
I think my bad habits of Anyway, I've switched all instances of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Apologies, thanks for all the work on this library and this looks neat, but is there any documentation on how to use it? I don't seem to find instructions on how to request "categories" in the documentation, yet I presume that any changes would have to be thoroughly documented to the user before a PR is merged. Where should I look? Also, what is the definition and criteria for |
@mborsetti I'm pretty sure they're listed in https://python-holidays.readthedocs.io/en/latest/api.html#holidays.holiday_base.HolidayBase So, for example, calling up the Californian-specific list for 2024 with holidays.country_holidays("US", subdiv="CA", years=2024, categories=("public","unofficial",)) We might want to update In terms of criteria, there are a bunch of similarities with the existing
Unless "National Fruitcakes Toss Day" is celebrated with some events being held for it, I don't think it belongs in this category except for perhaps Manitou Springs, Colorado if we ever get to add that subdivision in once #1713 is implemented. |
Hey Mike, long time no see! It's great you're still keeping an eye on the PH. I appreciate your reminder regarding keeping the docs up to date (which is not always the case). Let's look into your concerns in details:
The usage of categories mentioned here and in the Holiday categories support section. All supported categories specified on per entity basis and listed in the Available Countries table (Supported Categories column). What documentation is missing is description of each category (I'll create an issue to address that). This leads us to your next concern
As there is no standard for holiday categories I trust other fellow contributors/maintainer with their choice unless I clearly notice something is not fit. Normally we resolve this kind of concerns during code reviews or in issues. My general observation is that PH could use a better implemented/documented code ratio. Neither of the current maintainers are native English speakers. But if you want to blame somebody for that -- it'd be me. I'm responsible for this level of documentation quality (normally I review and approve every PR merged into dev/main branch -- btw, we moved to the new branch naming recently!). You can see @PPsyrius's response above with his idea of I also think it's worth to mention that I've started planning PH v1 roadmap recently. It has a separate issue for better documentation. If you have any suggestions/ideas please contribute. |
It's handled here. The default category is |
Hey Arkadii, Thanks for your response, and great leadership! I do get notices of new releases and when I have time check in on all the great improvements; wonderful job, I can't believe how many countries/subdivisions are supported now!
Wow, wonderful to see; the API does indeed need rethinking IMHO. I see if I have time to do a deeper dive. |
That's right! A lot of people contributed to PH's success. However, personally I'm really proud of the quality of the data provided by PH. @KJhellico and @PPsyrius have been doing amazing work on hunting down every single holiday in their contributions. |
UNOFFICIAL
category for the United States.UNOFFICIAL
category for the United States
Proposed change
This PR implements additional holidays (marked as
UNOFFICIAL
category), which includes:Closes #1643 .
Type of change
Checklist
make pre-commit
, it didn't generate any changesmake test
, all tests passed locally